Skip to content

Move co-located containers onto shared foreman-core-network - #728

Open
Alleny244 wants to merge 1 commit into
theforeman:masterfrom
Alleny244:fix/postgresql-listen-localhost
Open

Move co-located containers onto shared foreman-core-network#728
Alleny244 wants to merge 1 commit into
theforeman:masterfrom
Alleny244:fix/postgresql-listen-localhost

Conversation

@Alleny244

@Alleny244 Alleny244 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Problem Statement

Embedded PostgreSQL defaults to listen_addresses = '*', exposing port 5432 on public interfaces when containers use host networking.

https://redhat.atlassian.net/browse/SAT-48503

Changes

  • Add a shared Podman bridge network foreman-core-network (10.130.0.0/24) via the foreman_core_network role, created early in deploy.
  • Move co-located containers off host networking onto that network (PostgreSQL, Valkey, Candlepin, Pulp, Foreman/Dynflow, IOP services).
  • Reach services by container name on the shared network (e.g. postgresql, valkey, candlepin, iop-core-gateway).
  • Publish host access on loopback only where needed (e.g. 127.0.0.1:5432, 127.0.0.1:6379, Candlepin/IOP gateway ports) so ports are not exposed on 0.0.0.0 / public interfaces.
  • Remove the IOP-only iop_network / iop-core-network; IOP uses foreman-core-network.
  • Update TLS SANs and Smart Proxy URL so Foreman can reach Candlepin and the IOP gateway by container name.
  • Leave foreman_proxy on host networking (edge/DHCP/DNS/TFTP/REX).
  • Add/update tests to ensure PostgreSQL (and Valkey) are not published on 0.0.0.0:5432 / [::]:5432 (and equivalent for Valkey).

Tests

./forge test --pytest-args="tests/postgresql_test.py -vv"

Checklist

  • Tests added/updated
  • Documentation updated (docs/iop.md)

@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch from c6c2b37 to de1fe85 Compare August 5, 2026 14:19
@Alleny244
Alleny244 marked this pull request as draft August 5, 2026 14:22
@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch 4 times, most recently from 6bf8eb0 to a7edb41 Compare August 6, 2026 09:16
@Alleny244
Alleny244 marked this pull request as ready for review August 6, 2026 11:47
Comment thread src/roles/iop_core/tasks/main.yaml Outdated
@ehelms

ehelms commented Aug 7, 2026

Copy link
Copy Markdown
Member

Here's two other ideas:

  1. Put all the containers on the same network.

The iop network was created back in the foreman-installer for isolation, to reduce the need for TLS everywhere and because we were learning. Then we copied the idea here to start with parity. However, back when we implemented iop we always said this should get easier to manage once everything is in containers because we can normalize.

  1. Put postgresql on both networks and have it listen to both.

@Alleny244

Copy link
Copy Markdown
Contributor Author

Here's two other ideas:

  1. Put all the containers on the same network.

The iop network was created back in the foreman-installer for isolation, to reduce the need for TLS everywhere and because we were learning. Then we copied the idea here to start with parity. However, back when we implemented iop we always said this should get easier to manage once everything is in containers because we can normalize.

  1. Put postgresql on both networks and have it listen to both.

I’ll go with PostgreSQL on both networks here. Putting all containers on the same network would require broader changes, so a separate PR would be right?

@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch 5 times, most recently from 3b4d9d7 to 6f86341 Compare August 7, 2026 11:47
@ehelms

ehelms commented Aug 7, 2026

Copy link
Copy Markdown
Member

I’ll go with PostgreSQL on both networks here. Putting all containers on the same network would require broader changes, so a separate PR would be right?

Yes and no, you are ultimately trying to solve this broader problem of running only what's needed on the public interface. I'd rather us solve the architecture question than patch it and change it again right after.

I was reading on how pods accomplish the networking issue, and they create a lightweight "infra" container that basically does nothing but is the first container to always start and is present on the network to ensure it's available. I do not think we should use pods here as that will complicate things, but perhaps we can learn from this idea. The trickiest part would be getting the ordering right. But I do think if they are all on the same network, things get easier and I do not see a reason anymore to split them across networks.

@Alleny244

Copy link
Copy Markdown
Contributor Author

I’ll go with PostgreSQL on both networks here. Putting all containers on the same network would require broader changes, so a separate PR would be right?

Yes and no, you are ultimately trying to solve this broader problem of running only what's needed on the public interface. I'd rather us solve the architecture question than patch it and change it again right after.

I was reading on how pods accomplish the networking issue, and they create a lightweight "infra" container that basically does nothing but is the first container to always start and is present on the network to ensure it's available. I do not think we should use pods here as that will complicate things, but perhaps we can learn from this idea. The trickiest part would be getting the ordering right. But I do think if they are all on the same network, things get easier and I do not see a reason anymore to split them across networks.

Yeah, makes sense. I’ll look into the single network approach and see how we can handle the startup ordering.

@Alleny244

Copy link
Copy Markdown
Contributor Author

I’ll go with PostgreSQL on both networks here. Putting all containers on the same network would require broader changes, so a separate PR would be right?

Yes and no, you are ultimately trying to solve this broader problem of running only what's needed on the public interface. I'd rather us solve the architecture question than patch it and change it again right after.

I was reading on how pods accomplish the networking issue, and they create a lightweight "infra" container that basically does nothing but is the first container to always start and is present on the network to ensure it's available. I do not think we should use pods here as that will complicate things, but perhaps we can learn from this idea. The trickiest part would be getting the ordering right. But I do think if they are all on the same network, things get easier and I do not see a reason anymore to split them across networks.

Should I reuse the existing iop-core-network or create a new general purpose network ( if naming conventions are being looked into) for the containers

@ehelms

ehelms commented Aug 10, 2026

Copy link
Copy Markdown
Member

Should I reuse the existing iop-core-network or create a new general purpose network ( if naming conventions are being looked into) for the containers

New general purpose network name that is more descriptive.

@stejskalleos
stejskalleos self-requested a review August 11, 2026 07:57
Comment thread development/roles/foreman_development/defaults/main.yaml
Comment thread src/roles/iop_fdw/defaults/main.yaml Outdated
# Optional parameters - can use defaults.
# Use 127.0.0.1: Postgres is published on IPv4 only (not ::1), and TCP
# requires a password now that it is no longer on host networking.
iop_fdw_database_host: "127.0.0.1"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iop.md architecture diagram (mermaid) still draws Postgres under "Host System", outside the iop-core-network subgraph, while the prose now says Postgres is reached via postgresql:5432 on that network.

Should we update that as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's updated now

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do some places use 127.0.0.1 and others use postgresql ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alleny244 this should be addressed, as you use both in IOP.md:

flowchart LR
    Ansible["Ansible / tests on host"] -->|"127.0.0.1:5432<br/>published"| PG
    Apps["Foreman, Pulp, IOP apps"] -->|"postgresql:5432<br/>bridge DNS"| PG
    FDW["postgres_fdw inside PG"] -->|"127.0.0.1:5432<br/>container loopback"| PG
    PG[(postgresql container)]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, addressed this in the docs. The difference is the network context: host-side Ansible/tests use the published 127.0.0.1:5432, while the app containers use postgresql:5432 through the bridge. The FDW runs inside the PostgreSQL container, so its 127.0.0.1 refers to the PostgreSQL container itself. I’ve added a note and linked the “PostgreSQL from three vantage points” section.

Comment thread src/roles/iop_fdw/tasks/main.yaml
Comment thread src/vars/database.yml
Comment thread src/vars/database.yml Outdated
Comment thread src/roles/postgresql/tasks/main.yml
@Alleny244 Alleny244 changed the title Restrict PostgreSQL listen_addresses to localhost and IOP gateway Move co-located containers onto shared foreman-core-network Aug 11, 2026
@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch from 6f86341 to d65b220 Compare August 11, 2026 10:53
@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch 2 times, most recently from 5c5760a to 456b6e3 Compare August 11, 2026 13:16
@Alleny244

Copy link
Copy Markdown
Contributor Author

Can you introduce a new document that lays out the architecture of the network? Take a look at #771 as I'd like it to go into that folder structure.

Sure I ll take a look into that and formulate one

@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch from 0e55232 to 4c367a1 Compare August 21, 2026 11:32
@Alleny244

Copy link
Copy Markdown
Contributor Author

One thing that I noticed was , TLS issue affects both Candlepin and IOP: their certs are issued for candlepin / iop-core-gateway, but deploy-dev Foreman connects via localhost. Currently we are using those hostnames instead of localhost so hostname verification succeeds for production. For the host Foreman, I guess we’d also need to add both names to /etc/hosts pointing to 127.0.0.1 for TLS

@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch 5 times, most recently from fab18cb to 8766272 Compare August 22, 2026 18:33
@ehelms

ehelms commented Aug 23, 2026

Copy link
Copy Markdown
Member

One thing that I noticed was , TLS issue affects both Candlepin and IOP: their certs are issued for candlepin / iop-core-gateway, but deploy-dev Foreman connects via localhost. Currently we are using those hostnames instead of localhost so hostname verification succeeds for production. For the host Foreman, I guess we’d also need to add both names to /etc/hosts pointing to 127.0.0.1 for TLS

For devel setup, could the certificates be issued for localhost as they are today?

Comment thread development/playbooks/deploy-dev/deploy-dev.yaml Outdated
Comment thread src/roles/candlepin/defaults/main.yml Outdated
@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch from 8766272 to 6961194 Compare August 24, 2026 09:36
@Alleny244

Copy link
Copy Markdown
Contributor Author

One thing that I noticed was , TLS issue affects both Candlepin and IOP: their certs are issued for candlepin / iop-core-gateway, but deploy-dev Foreman connects via localhost. Currently we are using those hostnames instead of localhost so hostname verification succeeds for production. For the host Foreman, I guess we’d also need to add both names to /etc/hosts pointing to 127.0.0.1 for TLS

For devel setup, could the certificates be issued for localhost as they are today?

Yeah it is covered now. The Candlepin and IOP certs include both the container names (candlepin / iop-core-gateway) and localhost as SANs. So production can connect using the container names, while devel setup can continue using localhost (no /etc/hosts changes are needed)

@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch from 6961194 to 142b2ad Compare August 25, 2026 10:21

@stejskalleos stejskalleos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some things has to be addressed before we can merge.

Also, the failing CI:

TASK [pre_install : Install other dependencies] ********************************
[ERROR]: Task failed: Module failed: Depsolve Error occurred: 
 Problem: package python3.12-pyOpenSSL-26.2.0-2.el10.noarch from pulpcore requires (python3.12dist(cryptography) < 49~~ with python3.12dist(cryptography) >= 46), but none of the providers can be installed
  - package python3.12-requests-2.33.1-3.el10.noarch from pulpcore requires python3.12-pyOpenSSL >= 0.14, but none of the providers can be installed
  - cannot install both python3-cryptography-49.0.0-1.el10.x86_64 from baseos and python3-cryptography-48.0.0-3.el10.x86_64 from baseos
  - package python3-cryptography-49.0.0-1.el10.x86_64 from baseos obsoletes python3.12-cryptography < 49.0.0-1.el10 provided by python3.12-cryptography-46.0.7-3.el10.x86_64 from pulpcore
  - 

do we track it somewhere? Or is there a fix / person working on it?

Comment thread src/roles/certificates/tasks/issue.yml Outdated
Comment thread src/roles/iop_fdw/defaults/main.yaml Outdated
# Optional parameters - can use defaults.
# Use 127.0.0.1: Postgres is published on IPv4 only (not ::1), and TCP
# requires a password now that it is no longer on host networking.
iop_fdw_database_host: "127.0.0.1"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alleny244 this should be addressed, as you use both in IOP.md:

flowchart LR
    Ansible["Ansible / tests on host"] -->|"127.0.0.1:5432<br/>published"| PG
    Apps["Foreman, Pulp, IOP apps"] -->|"postgresql:5432<br/>bridge DNS"| PG
    FDW["postgres_fdw inside PG"] -->|"127.0.0.1:5432<br/>container loopback"| PG
    PG[(postgresql container)]

Comment thread src/vars/database.yml
Comment thread tests/feature/katello/candlepin_test.py Outdated
Comment on lines +55 to +56
"podman exec foreman curl --cacert /etc/foreman/katello-default-ca.crt"
"--silent --output /dev/null https://candlepin:23443/candlepin/status"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"podman exec foreman curl --cacert /etc/foreman/katello-default-ca.crt"
"--silent --output /dev/null https://candlepin:23443/candlepin/status"
"podman exec foreman curl --cacert /etc/foreman/katello-default-ca.crt "
"--silent --output /dev/null https://candlepin:23443/candlepin/status"

missing space, this can't work IMO

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, thanks, have added the space

Comment thread tests/feature/katello/candlepin_test.py Outdated
server.run(
f"curl --cacert {certificates['ca_certificate']} --silent --output /dev/null "
f"https://localhost:23443/candlepin/status"
f"podman exec foreman curl --cacert {certificates['ca_certificate']} "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From AI:

certificates['ca_certificate'] resolves to a host-side path (rendered from src/vars/certificates.yml, e.g.,/var/lib/foremanctl/certs/certs/ca.crt), but this is run with podman exec foreman, i.e., inside the foreman container's filesystem, where that path doesn't exist.

The sibling test test_candlepin_status correctly uses the in-container path /etc/foreman/katello-default-ca.crt — this one should too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeahh, changed!!

Comment thread src/playbooks/deploy/deploy.yaml Outdated
foreman_database_host: "{{ database_host }}"
candlepin_database_host: "{{ database_host }}"
pulp_database_host: "{{ database_host }}"
when: database_mode == 'external'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed here now? If the user provides the parameter this should cascade to modifying these in the src/vars/database.yml file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah agreed, have moved it back to the database.yml file

- name: Set external database hosts
ansible.builtin.set_fact:
pulp_database_host: "{{ database_host }}"
when: database_mode == 'external'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not support external database with the deploy-proxy action.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have removed it

Comment thread src/vars/base.yaml Outdated
candlepin_ca_certificate: "{{ ca_certificate }}"
candlepin_tomcat_key: "{{ localhost_key }}"
candlepin_tomcat_certificate: "{{ localhost_certificate }}"
candlepin_tomcat_key: "{{ candlepin_key }}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to remember why we set any of these certificate things here and not in src/vars/certificates.yaml.

Comment thread src/vars/certificates.yml Outdated
localhost_client_key: "{{ certificates_ca_directory }}/private/localhost-client.key"
localhost_client_certificate: "{{ certificates_ca_directory }}/certs/localhost-client.crt"
candlepin_key: "{{ certificates_ca_directory }}/private/candlepin.key"
candlepin_certificate: "{{ certificates_ca_directory }}/certs/candlepin.crt"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are a bit confusing to name this way and not use tomcat in the name. Since these are for tomcat whereas Candlepin itself does get key/cert that are CAs for it's other use cases. I would align on tomcat naming. And see https://github.com/theforeman/foremanctl/pull/728/changes#r3872587738 I think we need some alignment of certs vars into the same file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeahh, makes sense have made the change

Comment thread src/vars/database.yml
postgresql_admin_password_file: "{{ obsah_state_path }}/postgresql-admin-password"
postgresql_admin_password: "{{ lookup('ansible.builtin.password', postgresql_admin_password_file, chars=['ascii_letters', 'digits']) }}"

candlepin_database_host: "{{ database_host }}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we dropping these here?

@Alleny244 Alleny244 Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those variables used to point to localhost but with new shared network I thought of keep them under their respective roles default variables, but based on this feedback(#728 (comment)) I think it is better to have it in the initial place itself

Comment thread src/vars/database.yml Outdated
@@ -1,5 +1,5 @@
---
database_host: localhost
database_host: 127.0.0.1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought this would be postgresql now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this stay as 127.0.0.1? database_host is used for host side tasks(login_host, pg_isready), while the containers use their own *_database_host defaults with postgresql on foreman-core-network.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a fair question as database_host now has two meanings with this change. I believe it should mean "this is the host that services find that database at". In an external database scenario, this should represent the host of the external database. I would turn it around then, and consider if a new variable is needed to represent the host where database tooling connects to.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe like database_management_host ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this makes sense. database_host should represent the host that the services connect to, and we can use database_management_host for host side database tooling. I ll update the pr

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!!

@stejskalleos stejskalleos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments have been addressed; running foremanctl deploy works fine, and CI is green.

🍏 LGTM from my side, waiting for @ehelms for final ack

@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch from ecd85ee to 5ede7ef Compare September 1, 2026 06:02
@Alleny244
Alleny244 force-pushed the fix/postgresql-listen-localhost branch from 5ede7ef to 07b8960 Compare September 1, 2026 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants